-
Notifications
You must be signed in to change notification settings - Fork 6
feat: create db_dtypes JSONDtype and JSONArray #284
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not quite finished with the review, but sending comments now since I have a meeting and want to share what I have so far before I get back to it.
db_dtypes/json.py
Outdated
| @property | ||
| def type(self) -> type[str]: | ||
| """Return the scalar type for the array, e.g. int.""" | ||
| return dict |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be a Union[dict, list, str, int, float]?
Please create a JSONScalarType = Union[dict, list, str, int, float] module-level variable and use it here, if so.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried JSONScalarType and typing.Any but both of them are hitting errors on pandas source codes. Also, I didn't find an example Extension dtype that has multiple types. For now, I would leave it as a str to indicate its storage type and the type of to_numpy as well. Here are the call stack of both of them:
https://screenshot.googleplex.com/6GiKkNU9T2e8GPm
https://screenshot.googleplex.com/9QfDFsrUzD7PMrT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another round of comments before I go to lunch. Thanks for your patience with the review.
5b8f379 to
9e92dc7
Compare
cebde95 to
efe72cc
Compare
865f93b to
790f257
Compare
98debff to
b4cfcd9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once test coverage is reached.
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes internal bug b/312728178 🦕